Repository navigation
fix(runtime): retry worker and widget-dev cleanup with the promise form of rm - #631
Merged
Merged
Conversation
…rm of rm On Windows, rmSync reports a held directory as EBUSY or EPERM at once and never runs its maxRetries, so the worker's run directory and a superseded widget-dev snapshot were left behind whenever something still held them. Both callers are already async, so they now await rm from node:fs/promises, which does retry, without blocking the event loop. The widget-dev prune runs on the session's chain, so nothing else of that session interleaves. Tests hold each directory with a child process's working directory until 300 ms after its removal starts; they fail against the old rmSync calls.
Contributor
Author
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
mrgoonie
enabled auto-merge (squash)
October 7, 2026 22:16
Contributor
Author
|
Review attestation: ready to merge at A push to this PR makes this attestation stale; the new head needs its own review. |
This was referenced Oct 8, 2026
mrgoonie
added a commit
that referenced
this pull request
Oct 8, 2026
… a bound (#646) `WidgetDevSessions.close()` now returns a promise that waits for every session's queued work, including a superseded snapshot still being removed with retries, for at most two seconds. The node's shutdown starts it with the other stops and awaits it before the database closes. The spec awaits `close()` and removes its folder with the async cleanup helper. The held-snapshot test no longer races a 300 ms timer against the retry budget: one attempt meets the hold, the holder exits, and then the runtime's own removal runs, whose retry options are asserted. Refs #626, #631 Fixes #639
This was referenced Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #626. Refs #563, #622.
Why
On Windows,
rmSync(path, { recursive: true, force: true, maxRetries })reports a held directory asEBUSY(Node 22) orEPERM(Node 24) at once and never runs its retries. #622 measured this, and it reproduces here (Windows 11, Node v24.11.0): with a child process holding the directory as its working directory,rmSync(..., { maxRetries: 5, retryDelay: 100 })threwEPERMafter 0 ms, whileawait rm(...)fromnode:fs/promiseswith the same options removed it after 637 ms, once the holder let go 300 ms in.Two production call sites relied on the broken retry:
apps/runtime/src/worker-process.ts(runWorkerProcess'sfinally)rmSync(directory, { ..., maxRetries: 5, retryDelay: 100 })await rm(directory, { ..., maxRetries: 5, retryDelay: 100 })apps/runtime/src/application/widget-dev-sessions.ts(prune)rmSync(path, { ..., maxRetries: 2 })await rm(path, { ..., maxRetries: 5, retryDelay: 100 })What changes
runWorkerProcessis alreadyasync, so itsfinallynow awaitsrm. A failure is still swallowed as before (the directory holds the brief, never a key). The run's result or error now waits for cleanup: at most about 1.5 s, and only while the directory is held.prunebecomesasync, and both callers await it. Both callers (followandactivate) are alreadyasync, and they run only on the session'sserialchain, so nothing else of that session interleaves with the removal. A snapshot that still cannot be removed after the retries stays on the session's list and is tried again after the next install, as before.retryDelay: 100(about 1.5 s), the same as the worker. The wait now lands on one session's chain, not on the event loop.exithandler, and no synchronous caller had to become async.createWidgetDevSessions().close()does not wait for the chains today, and it still does not.rmfromnode:fs/promisesdirectly, astask-browser.tsandpackage-fetch.tsalready do. No new helper is added.Tests
apps/runtime/test/hold-directory.tsis a new helper. It holds a directory by running a child process with that directory as its working directory, and it reports when the hold is really in place. On Windows, that makes removal fail withEBUSYorEPERMuntil the child exits. On other platforms the hold changes nothing, so the tests pass there but only tell the two forms apart on Windows.Each spec wraps
rmSyncandrmso that it can see when the removal starts, without changing what the removal does. It releases the hold 300 ms later. Only a removal that really retries finds the directory free.worker-process.spec.ts: removes its run directory even when the directory is still held for a moment after the worker exits. The hold starts when the run directory is made, and the test asserts that the hold was in place when the removal started, so it cannot pass without proving anything.widget-dev-sessions.spec.ts: removes a superseded snapshot that is held for a moment, as a file still open on Windows holds it. The first build's snapshot is held. The second build keeps it as the rollback target, and the third prunes it. The snapshot must be gone, and off the session's list.Both new tests fail against
main's source (Windows 11, Node v24.11.0). Each was run with only the source file reverted toorigin/main. Each fails atexpect(existsSync(...)).toBe(false), because the directory is still there.Verification (Windows 11, Node v24.11.0)
worker-process.spec.ts+widget-dev-sessions.spec.ts, 3 runs in a rowmain'sworker-process.tsmain'swidget-dev-sessions.tspnpm typecheckpnpm invariantseslinton the changed filespnpm verifypacks/browser-playwright/test/close-during-launch.spec.ts, whoseafterAllthrewEPERMfromrmSync(dir, { ..., maxRetries: 10 }). That is the test-cleanup bug #622 fixes, in a file this PR does not touch. Run again on its own, it passed 1/1.Overlap with open PRs
tools/invariants/test-cleanup-retries-asynchronously.mjs, which this PR does not touch. The two PRs do not change the same files. The new specs here usermSyncwithoutmaxRetries(in the specs' own wrappers and in the worker test's final cleanup), so they do not trip test: remove spec directories with the retrying async helper, since rmSync never retries on Windows #622's invariant. Extending that invariant (or a sibling) to production code, as fix(runtime): production cleanup uses rmSync with maxRetries, which never retries on Windows #626 suggests, can follow once test: remove spec directories with the retrying async helper, since rmSync never retries on Windows #622 lands, since no production site would fail it after this PR.apps/runtime/src/application/widget-dev-sessions.tsandapps/runtime/test/widget-dev-sessions.spec.ts. This PR touches onlyprune, its two call sites, thenode:fsmock at the top of the spec, and one new test after the existing prune test. Whichever lands second may need a small textual rebase.Overlap correction
#622 also edits
apps/runtime/test/widget-dev-sessions.spec.ts. A trial merge conflicts on one import line only; keeping both imports passes invariants (15/15) and the affected specs. Whichever of #612 or #614 lands after this PR must keepawait prune(...)at both call sites, because lint does not flag floating promises.